Skip to content

Conversation

@thomasjpfan
Copy link
Contributor

This PR cherry picks files from the following PRs to just implement the dynamic directory mounts for Go:

This includes the task command router. I think this is a smaller change and does not impact any public API around sb.exec. Once we have this in, then #242 is easier to review.

@cursor
Copy link

cursor bot commented Jan 26, 2026

PR Summary

Medium Risk
Introduces a new sandbox lifecycle contract (Detach) and changes the Sandbox.Terminate signature, plus adds a new gRPC command-router client with custom retry/auth refresh logic; mistakes here could cause resource leaks or break sandbox interactions.

Overview
Adds a new Sandbox.Detach() lifecycle step in Go and makes Sandbox.Terminate(ctx, detach, params) optionally detach after termination; all sandbox operations now fail with a new SandboxDetached error once detached.

Introduces an internal TaskCommandRouterClient (JWT auth + refresh, transient retry) and wires it into Sandbox to support experimental dynamic directory workflows: Sandbox.ExperimentalMountImage(path, image) and Sandbox.ExperimentalSnapshotDirectory(path).

Updates gRPC client connection window sizing, refreshes examples/tests/docs/changelog for the new terminate/detach API, and adds a new sandbox image-mount example plus coverage for detach behavior and mount/snapshot APIs.

Written by Cursor Bugbot for commit a07297a. This will update automatically on new commits. Configure here.

"google.golang.org/protobuf/types/known/emptypb"
)

// tlsCredsNoALPN is a TLS credential that skips ALPN enforcement, implementing
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this whole thing should no longer be needing after @saltzm fixed something server side, so might work if you just remove it and use the default?

@thomasjpfan thomasjpfan changed the title sandbox: dynamic directory mount & snapshot fully - Go sandbox: dynamic directory mount & snapshot in go + adds detach Jan 27, 2026
@thomasjpfan
Copy link
Contributor Author

Updated PR with:

  • Remove the ALPN workaround and verify that taking to the TCR still works
  • Adds Detach back to both typescript and go

@thomasjpfan
Copy link
Contributor Author

thomasjpfan commented Jan 27, 2026

I updated PR:

  • Disable almost every operation after Detach and added test for this behavior* (Details below)
  • Removed the JS changes, the PR is already getting too big. I'll open a smaller PR to add Detach to JS.

While implementing this, I made some decisions that are debatable:

  • Terminate is still allowed after Detach. It feels a little weird to not be able to terminate the sandbox after detach.
    • I'm open to changing this.
  • Terminate does not mark the sandbox as detached, so you can still call sb.Wait after calling Terminate. Terminate will still close the task command routers.
    • An alternative is to allow Terminate , Wait and Poll for a detached sandbox and have Terminate mark the sandbox as detached.

Copy link

@saltzm saltzm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good mod the comment about task_command_router_insecure which isn't being used anymore in the python client

func (sb *Sandbox) ensureAttached() error {
if sb.detached.Load() {
return SandboxDetached{Exception: "Do not call Detach or Terminate until you are done with your sandbox in this session"}
return SandboxDetached{Exception: "Do not call Detach until you are done with your sandbox in this session"}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I might find this error message confusing if I do like sb.Exec() and get this back - I'll be like "why is it telling me not to call Detach, I'm calling Exec".

I'd expect something more like "Unable to perform operation on a detached sandbox" or "Operations on sandbox after Detach are not allowed" or some such thing

fmt.Printf("Started Sandbox: %s\n", sb.SandboxID)
defer func() {
if err := sb.Terminate(context.Background()); err != nil {
if err := sb.Terminate(context.Background(), true, nil); err != nil {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferred terminate fails after manual detach in examples

Medium Severity

Deferred Terminate calls fail because the target sandbox is already detached. This occurs when a sandbox is explicitly detached or terminated with detach=true before the deferred call executes, leading to a fatal log.

Additional Locations (1)

Fix in Cursor Fix in Web

codes.Canceled: {},
codes.Internal: {},
codes.Unknown: {},
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated retryable gRPC status codes map

Low Severity

commandRouterRetryableCodes is identical to retryableGrpcStatusCodes defined in client.go (lines 285-291). Both maps contain the exact same five gRPC status codes: DeadlineExceeded, Unavailable, Canceled, Internal, and Unknown. The existing retryableGrpcStatusCodes variable could be reused instead of defining a duplicate.

Fix in Cursor Fix in Web


// SandboxTerminateParams are options for Terminate
type SandboxTerminateParams struct {
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused parameter and empty params struct

Low Severity

SandboxTerminateParams is an empty struct and the params parameter in Terminate() is never referenced in the function body. The parameter exists but serves no purpose in the current implementation.

Fix in Cursor Fix in Web

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.

closed atomic.Bool

refreshJwtGroup singleflight.Group
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exported types only used internally in package

Low Severity

TaskCommandRouterClient struct and TryInitTaskCommandRouterClient function are exported (uppercase names) but are only used within the modal package. These could be unexported to avoid polluting the public API surface.

Additional Locations (1)

Fix in Cursor Fix in Web

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants